nixos/power-profiles-daemon: Add assertion with auto-cpufreq #318342
Conversation
auto-cpufreq is similar to tlp in that it shouldn't be run with power-profiles-daemon. There functionality can conflict and bugs can show up. On my system this materialized by auto-cpufreq frequently shutting down, but there may be other consequences. This change follows the same pattern as the tlp assertion
github-actions
bot
added
6.topic: nixos
|
This is baby's first NixOS commit. I tried reading through documentation to make sure I'm following convention, but a few things were unclear. If I've done something wrong, please don't be afraid to tell me to RTFM and send me in the general direction of docs. The one thing that was least clear to me was how to test this change. Most of what I saw in documentation revolved around build/testing packages and not configuration changes. This assertion triggers on a pretty easy situation to get into, having gnome and auto-cpufreq both enabled. While there is no situation where a user would want that, I think this will probably still be considered a breaking change and should target a future release. I made my PR target master for this reason, but let me know if that's wrong. |
NixOSInfra
added
the
12. first-time contribution
|
Pinging @Technical27 since I think you might be the most relevant maintainer |
ofborg
bot
added
10.rebuild-darwin: 0
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4070 |
There was a problem hiding this comment.
Hey @stusmall, welcome!
The one thing that was least clear to me was how to test this change. Most of what I saw in documentation revolved around build/testing packages and not configuration changes.
Testing those assertions is pretty ad-hoc for now. I personally use the VM tests. So, in this occurence, I first did a:
$ nix-build -A nixosTests.power-profiles-daemonTo make sure this do not break anything, and it didn't. Then, applied:
diff --git a/nixos/tests/power-profiles-daemon.nix b/nixos/tests/power-profiles-daemon.nix
index 8a54d8e8bab8..b003dd69f828 100644
--- a/nixos/tests/power-profiles-daemon.nix
+++ b/nixos/tests/power-profiles-daemon.nix
@@ -8,6 +8,7 @@ import ./make-test-python.nix ({ pkgs, ... }:
nodes.machine = { pkgs, ... }: {
security.polkit.enable = true;
services.power-profiles-daemon.enable = true;
+ services.auto-cpufreq.enable = true;
environment.systemPackages = [ pkgs.glib pkgs.power-profiles-daemon ];
};Then, evaluating the test closure, you can see the assertion is correctly triggered:
[nix-shell:~/.cache/nixpkgs-review/pr-318342/nixpkgs]$ nix-build -A nixosTests.power-profiles-daemon
error:
… while evaluating the attribute 'drvPath'
at /home/ninjatrappeur/.cache/nixpkgs-review/pr-318342/nixpkgs/lib/customisation.nix:365:7:
364| in commonAttrs // {
365| drvPath = assert condition; drv.drvPath;
| ^
366| outPath = assert condition; drv.outPath;
… while calling the 'derivationStrict' builtin
at /builtin/derivation.nix:9:12: (source not available)
(stack trace truncated; use '--show-trace' to show the full trace)
error:
Failed assertions:
- You have set services.power-profiles-daemon.enable = true;
which conflicts with services.auto-cpufreq.enable = true;You can read this manual section to learn more about these VM tests: https://nixos.org/manual/nixos/stable/#sec-nixos-tests
The rest looks good. Spot on first contribution, nice work, glad to have you here :)
|
I really appreciate the detailed explanation of your testing steps! Thank you very much |
Description of changes
auto-cpufreq is similar to tlp in that it shouldn't be run with power-profiles-daemon. There functionality can conflict and bugs can show up. On my system this materialized by auto-cpufreq frequently shutting down, but there may be other consequences.
This change follows the same pattern as the tlp assertion
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.